Avoid allocations due to ConcurrentStack usage#12151
Merged
AR-May merged 1 commit intodotnet:mainfrom Aug 21, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the thread‐safe ConcurrentStack<GenericExpressionNode> pools with a plain Stack<GenericExpressionNode> guarded by a lock to eliminate per‐push node allocations and reduce GC pressure.
- Swaps all
ConcurrentStack<GenericExpressionNode>usage toStack<GenericExpressionNode>in the cache structure. - Adds a
lock(expressionPool)around popping/parsing logic inEvaluateConditionCollectingConditionedProperties. - Updates the
ExpressionTreeForCurrentOptionsWithSizeconstructor andGetOrAddsignatures to useStack<GenericExpressionNode>.
Comments suppressed due to low confidence (3)
src/Build/Evaluation/ConditionEvaluator.cs:251
- Add unit tests that simulate concurrent access to the new
Stack<GenericExpressionNode>pools, ensuring that both retrieval and return of parsed expressions under lock correctly preserve pool integrity and reuse.
lock (expressionPool)
src/Build/Evaluation/ConditionEvaluator.cs:251
- Only the pop-and-parse logic is wrapped by this lock, but the subsequent push back into the pool (still using
Stack.Push) occurs outside it. SinceStack<T>is not thread-safe, wrap both pop and push operations in the same lock to avoid race conditions.
lock (expressionPool)
src/Build/Evaluation/ConditionEvaluator.cs:255
- Declaring
parsedExpressioninside the lock block can limit its scope if you later need to use it outside. Consider moving the declaration immediately before thelockso its value is accessible throughout the remainder of the method.
GenericExpressionNode parsedExpression;
ghost
assigned
AR-May
approved these changes
Aug 20, 2025
Member
AR-May
left a comment
AR-May
left a comment
There was a problem hiding this comment.
Looks good to me. I ran exp insertion, let's see if it passes.
JanProvaznik
approved these changes
Aug 20, 2025
Member
JanProvaznik
left a comment
JanProvaznik
left a comment
There was a problem hiding this comment.
looks correct in the reasoning and code, let's see insertion
Member
|
exp insertion is good: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/663329 |
JanProvaznik
pushed a commit
that referenced
this pull request
Aug 26, 2025
The implementation of `ConcurrentStack<T>` creates a new wrapper node
each time an item is pushed onto the stack
public void Push(T item)
{
Node node = new Node(item);
node.m_next = m_head;
if (Interlocked.CompareExchange(ref m_head, node, node.m_next) !=
node.m_next)
{
PushCore(node, node);
}
}
The creates an appreciable amount of allocations during build.
These allocations can almost entirely be eliminated by using a vanilla
generic `Stack<T>` with a lock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #
Context
The implementation of
ConcurrentStack<T>creates a new wrapper node each time an item is pushed onto the stackThe creates an appreciable amount of allocations during build:
These allocations can almost entirely be eliminated by using a vanilla generic
Stack<T>with a lock.Changes Made
Testing
Notes